Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

./x.py test src/test/run-make passes for me locally, so I don't know why travis failed.

There's at least one span_to_filename usage in librustdoc that is still wrong. The problem with it is that the result is used for both input and display purposes, so the change to fix it will be more extensive, probably requiring passing around both the original path and the remapped name. I can work on that if this sounds like the right approach. There's a couple more span_to_filename uses in librustdoc that I haven't investigated fully.

Also, anywhere that converts strings to PathBuf seems error prone, since we don't know if the string is a real path, or if it was lossily converted from a path, or if it isn't a path at all. In this PR I've moved an occurrence of this from the callers of span_to_filename into new_filemap. I initially tried to avoid this completely, but that was going to be a much larger change to all the callers of new_filemap. Is this worth pursuing?

Thanks a lot for the PR, @philipc! I'll take a closer look shortly.

Looks like an excellent start! Some comments:

./x.py test src/test/run-make passes for me locally, so I don't know why travis failed.

This seems to be a spurious failure probably not related to your PR: #43402

There's at least one span_to_filename usage in librustdoc that is still wrong. The problem with it is that the result is used for both input and display purposes, so the change to fix it will be more extensive, probably requiring passing around both the original path and the remapped name. I can work on that if this sounds like the right approach. There's a couple more span_to_filename uses in librustdoc that I haven't investigated fully.

Could you just add a FIXME comment to those and link to this PR? I don't think it's possible to invoke rustdoc with path remapping anyway.

Also, anywhere that converts strings to PathBuf seems error prone, since we don't know if the string is a real path, or if it was lossily converted from a path, or if it isn't a path at all. In this PR I've moved an occurrence of this from the callers of span_to_filename into new_filemap. I initially tried to avoid this completely, but that was going to be a much larger change to all the callers of new_filemap. Is this worth pursuing?

Let's keep this PR focussed, I'd say. The other changes can still be done later.

Some minor comments regarding the implementation:

• I would prefer a different name for the path field, for example fs_path or unmapped_path -- something blunt :) (and CodeMap::span_to_path() should then also be named accordingly)
• Intended or not, this PR does the right thing for reproducible builds in that it does not serialize the path field into crate metadata. However, I'd prefer if the field was an Option<Path> to make this more explicit. span_to_path() can call .expect("CodeMap::span_to_path() called for imported file-map?") to make things easy.

Otherwise this looks great already!

I don't think it's possible to invoke rustdoc with path remapping anyway.

Is this something that should be possible though? Do distributions want to package the generated doc, and have the src links point to the remapped location?

I'll address the other comments, thanks for reviewing.

Is this something that should be possible though? Do distributions want to package the generated doc, and have the src links point to the remapped location?

I could imagine so. But since the current rustdoc implementation will be replaced by one that coupled less tightly to the compiler internals, I don't think we need to solve this here.

r? @michaelwoerister

I've updated FileMap::path as requested.

Also added a FIXME for doctests, but not sure how useful it is.

@bors r+

Excellent, thanks a lot, @philipc!

📌 Commit 9bbd7a3 has been approved by michaelwoerister

⌛ Testing commit 9bbd7a3 with merge a0db04b...

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing a0db04b to master...